Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add v1.13 semantic conventions #3499

Merged
merged 32 commits into from
Jan 5, 2023
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Nov 29, 2022

Fix #3191
Closes #3533

  • Generate semconv/v1.13
  • Replace generated Go functions that generate OTel attributes with ones that conform with the v1.13 semantic convetions specification
    • Add the semconv/v1.13/httpconv to provide HTTP trace conventions
    • Add the semconv/v1.13/netconv to provide net

The replacement of these previous functions comes from the refactor in the specification of the net and http semantic conventions (open-telemetry/opentelemetry-specification#2469, open-telemetry/opentelemetry-specification#2614), and a desire to make these convenience functions more ergonomic.

Open Question

I haven't included metric specific functions here. Namely because the difference is going to be in the http.route attribute on the server which is not generated in the span conventions. That means the current trace conventions can be used to comply with the metric conventions currently. I am not sure how we want to partition signal specific conventions in the future?

@MrAlias MrAlias mentioned this pull request Nov 29, 2022
@MrAlias MrAlias added this to the Release v1.12.0 milestone Nov 29, 2022
@MrAlias MrAlias marked this pull request as ready for review November 29, 2022 20:31
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #3499 (2fdc43e) into main (e368276) will increase coverage by 0.7%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3499     +/-   ##
=======================================
+ Coverage   77.9%   78.7%   +0.7%     
=======================================
  Files        163     165      +2     
  Lines      11850   12277    +427     
=======================================
+ Hits        9239    9666    +427     
  Misses      2413    2413             
  Partials     198     198             
Impacted Files Coverage Δ
semconv/internal/v2/http.go 100.0% <100.0%> (ø)
semconv/internal/v2/net.go 100.0% <100.0%> (ø)

Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change to templates backward compatible? By this, I mean both the API hasn't changed, and the functions don't have any additional outputs. I don't think it is needed, but it would be nice to have.

If it's not, or we don't want to test extensively enough to show it, we should probably add a warning not to generate anything prior to v1.13.0

semconv/v1.13.0/netconv/net.go Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 30, 2022

Is the change to templates backward compatible? By this, I mean both the API hasn't changed, and the functions don't have any additional outputs. I don't think it is needed, but it would be nice to have.

I'm not sure I follow.

The change produces different attributes as the specification requires different attributes. It is therefore not a backwards compatible change in accordance with the specification.

The template generation will not produce the old function sets. As we discussed in the SIG meeting a few weeks ago, instead of building complex logic to switch on an OTel spec version to build you can check out the old generation code if we wanted to generate prior versions, though I'm not sure why we want to do that though.

RELEASING.md Outdated Show resolved Hide resolved
@MadVikingGod
Copy link
Contributor

The template generation will not produce the old function sets.

I assumed this, but I wanted it to be extra clear. Can we add some documented warning that if you generate with spec version <v1.13.0 this will break things?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPClientAttributesFromHTTPRequest never sets http.flavor Generate the v1.13 version of the semconv pacakge
4 participants